Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitcoin native experience #371

Merged
merged 53 commits into from
May 29, 2024
Merged

Bitcoin native experience #371

merged 53 commits into from
May 29, 2024

Conversation

r-czajkowski
Copy link
Contributor

@r-czajkowski r-czajkowski commented Apr 18, 2024

Closes: #351

This PR integrates with the OrangeKit SDK to predict the depositor owner Ethereum address based on user's Bitcoin address. We want to transition to Bitcoin native experience in the SDK as well so we should operate on the bitcoin addresses only and hide the Ethereum integration under the hood. In the future, we will update the other function parameters to accept Bitcoin addresses instead of Ethereum.

Here we also introduce the BitcoinProvider interface which defines the getAddress method. We should rely on the BitcoinProvider implementation to get the Bitcoin address because different wallets use different strategies. For example, in Ledger Live the address is "renewed" each time funds are received in order to allow some privacy. In this case, relying only on the Bitcoin address, the user would create deposits for different Ethereum addresses each time the Bitcoin address was renewed.

Therefore, we implement the BitcoinProvider for Ledger Live Wallet API that it awalys returns the same Bitcoin address based on the extended public key (xpub). The Ledger Live Wallet API increments the index in the derivation path each time funds are received(m/x'/0'/0'/0/0; m/x'/0'/0'/0/1; m/x'/0'/0'/0/2 ...), so to get the same address we need to derive a single address from a given extended public key - an address under the m/x'/0'/0'/0/0 path. Thanks to this we can create a deposit for the same Ethereum address even if Ledger Live renewed the Bitcoin address.

To build Bitcoin-native experience.
Pass only bitcoin address to initialize the deposit process. Using
`@orangekit/sdk` we calculate the Ethereum address based on the Bitcoin
address. This created Ethereum address of the Safe should be used to
show the user's data like position, transaction history and activity
details.
Copy link

netlify bot commented Apr 18, 2024

Deploy Preview for acre-dapp-testnet ready!

Name Link
🔨 Latest commit 60c1231
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/665712c1f689240009052858
😎 Deploy Preview https://deploy-preview-371--acre-dapp-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sdk/src/modules/staking/index.ts Show resolved Hide resolved
sdk/src/modules/staking/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/staking/index.ts Show resolved Hide resolved
To get the bitcoin address for `x'/0'/0'/0/0` path of a given extended
public key to use always the same bitocin address as an identifier
because Leder Wallet API returns the "next" public address where a user
should receive funds. In the context of Bitcoin, the address is
"renewed" each time funds are received. We need to use the same bitcoin
address to generate always the same deposit owner address on ethereum.
We want to define an interface for communication with Bitcoin wallet and
use it in Acre SDK. Currently it defines `getAddress` function because
we need the bitcoin address to initialize the deposit.
Implements the `getAddress` function which always returns the same
bitcoin address based on the extended public key.
Use mixin pattern to share abstract logic but for different base
classes.
We want to relay on the `BitcoinProvider` implementation to get the
bitcoin address because different wallets use different strategies. For
example in Ledger Live the address is "renewed" each time funds are
received in order to allow some privacy. In that case always getting the
same bitcoin address is hidden under a given implementation.
Cover a case when the custom bitcoin recovery address is passed to
initialize deposit.
@r-czajkowski r-czajkowski requested a review from nkuba May 9, 2024 12:22
@r-czajkowski r-czajkowski added the 🔌 SDK TypeScript SDK Library label May 9, 2024
@r-czajkowski r-czajkowski marked this pull request as ready for review May 9, 2024 12:22
r-czajkowski and others added 6 commits May 9, 2024 14:27
Explain in the docs that this property is available to let the consumer
use `P2SH-P2WPKH` as the deposit owner and another tBTC-supported type
(`P2WPKH`, `P2PKH`) address as the tBTC Bridge recovery address.
`depositor` -> `depositorOwnerBitcoinAddress` - let's avoid `naming it
`depositor` so we don't confuse it with the `BitcoinDepositor` contract.
According to the [jest
documentation](https://jestjs.io/docs/getting-started#using-typescript)
there are two ways to get TypeScript to work with Jest:
- babel
- ts-jest

We already use ts-jest preset in jest configuration, so we don't need to
import babel presets.
We use the JestConfigWithTsJest type to be explicit about the type of
the config object we're dealing with.
After we added @orangekit/sdk import Jest started to complain with:
```
Jest encountered an unexpected token

Details:

/Users/jakub/workspace/acre/acre/node_modules/.pnpm/@orangekit[email protected]/node_modules/@orangekit/sdk/dist/src/index.js:1
({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export * from "./lib";
                                                                                  ^^^^^^

SyntaxError: Unexpected token 'export'

> 1 | import { OrangeKitSdk } from "@orangekit/sdk"
    | ^
  2 | import { JsonRpcProvider } from "ethers"
  3 | import { AcreContracts } from "./lib/contracts"
  4 | import { EthereumNetwork, getEthereumContracts } from "./lib/ethereum"

  at Runtime.createScriptFromCode (../node_modules/.pnpm/[email protected]/node_modules/jest-runtime/build/index.js:1505:14)
  at Object.<anonymous> (src/acre.ts:1:1)
  at Object.<anonymous> (src/index.ts:9:1)
  at Object.<anonymous> (test/modules/tbtc/Tbtc.test.ts:7:1)
```

The `@orangekit/sdk` module exports `dist/` directory containing JS files in ESM syntax.
Jest doesn't support ESM syntax, so we have to convert these JS files to
CommonJS syntax with `ts-jest/presets/js-with-ts` preset.
We have to define `transformIgnorePatterns` property as `node_modules/`
directory is excluded from transformations by default.

We also had to add `allowJs: true` in tsconfig.json to support
transformation of JS files.

Reference:
- https://stackoverflow.com/questions/75452411/why-isnt-jest-handling-es-module-dependencies
- https://stackoverflow.com/questions/49263429/jest-gives-an-error-syntaxerror-unexpected-token-export?noredirect=1&lq=1
Depends on: #417

After we added @orangekit/sdk import Jest started to complain with:
```
Jest encountered an unexpected token

Details:

/Users/jakub/workspace/acre/acre/node_modules/.pnpm/@orangekit[email protected]/node_modules/@orangekit/sdk/dist/src/index.js:1
({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export * from "./lib";
                                                                                  ^^^^^^

SyntaxError: Unexpected token 'export'

> 1 | import { OrangeKitSdk } from "@orangekit/sdk"
    | ^
  2 | import { JsonRpcProvider } from "ethers"
  3 | import { AcreContracts } from "./lib/contracts"
  4 | import { EthereumNetwork, getEthereumContracts } from "./lib/ethereum"

  at Runtime.createScriptFromCode (../node_modules/.pnpm/[email protected]/node_modules/jest-runtime/build/index.js:1505:14)
  at Object.<anonymous> (src/acre.ts:1:1)
  at Object.<anonymous> (src/index.ts:9:1)
  at Object.<anonymous> (test/modules/tbtc/Tbtc.test.ts:7:1)
```

The `@orangekit/sdk` module exports `dist/` directory containing JS
files in ESM syntax.
Jest doesn't support ESM syntax, so we have to convert these JS files to
CommonJS syntax with `ts-jest/presets/js-with-ts` preset.
We have to define `transformIgnorePatterns` property as `node_modules/`
directory is excluded from transformations by default.

We also had to add `allowJs: true` in tsconfig.json to support
transformation of JS files.

Unfortunately, it increased test execution time, so it may be better to
mock the `@orangekit/sdk` in unit tests instead of transforming the
code.

Reference:
-
https://stackoverflow.com/questions/75452411/why-isnt-jest-handling-es-module-dependencies
-
https://stackoverflow.com/questions/49263429/jest-gives-an-error-syntaxerror-unexpected-token-export?noredirect=1&lq=1
dapp/src/acre-react/hooks/useStakeFlow.ts Outdated Show resolved Hide resolved
sdk/package.json Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Show resolved Hide resolved
sdk/src/typings.d.ts Outdated Show resolved Hide resolved
sdk/src/lib/bitcoin/providers/provider.ts Show resolved Hide resolved
sdk/src/modules/staking/index.ts Outdated Show resolved Hide resolved
For security reasons. In the future, we should probably fork it or copy
that we need to resolve the addresses from `xpub`.
We should avoid prefixing constructor args with `_` as it may suggest
that the argument should be ignored, but we use it.
Define two separate functions `initializeMainnet` and
`initializeTestnet` so consumers won't pass the network param
unnecessarily.
Create `VoidSigner` compatible with ethers v5. In the future we are
going to use the ethers signer from `@orangekit/sdk` under the hood.
This signer is used only interanlly by the Acre SDK to interact with
Ethereum contracts.
If there is one export we should use it as default.
r-czajkowski and others added 5 commits May 15, 2024 15:00
Reverts 553b5be commit. We can just
mock the `@orangekit/sdk` module globally to run jest tests correctly
w/o converting the JS files to CommonJS syntax.
Depends on: #371

This PR removes the Ethereum account from the dapp context to implement
Bitcoin native experience. Since the signing Bitcoin messages doesn't
work in the Ledger Live Wallet API we temporarily removed this step from
the deposit flow.
Remove unnecessary `ETHEREUM_NETWORK` param. The ethereum network is no
longer needed to create the Acre SDK.
sdk/src/acre.ts Outdated Show resolved Hide resolved
dapp/src/hooks/useWallet.ts Show resolved Hide resolved
sdk/src/modules/staking/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/tbtc/Tbtc.ts Show resolved Hide resolved
sdk/src/lib/utils/ethereum-signer.ts Outdated Show resolved Hide resolved
@r-czajkowski r-czajkowski mentioned this pull request May 16, 2024
`ethereumAddress` -> `depositOwnerEvmAddress`
Fix docs for the `bitcoinProvider` property.
Extends the `Signer` interface from ethers v6 to include more methods
required by ethers v5.
@r-czajkowski r-czajkowski requested a review from nkuba May 21, 2024 14:52
dapp/src/constants/chains.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/lib/bitcoin/providers/provider.ts Show resolved Hide resolved
sdk/src/modules/staking/index.ts Outdated Show resolved Hide resolved
Instead of exporting the BitcoinNetwork we import from tBTC SDK we
define it with the networks we support - mainnet and testnet.
Remove the `initializeMainnet` and `initializeTestnet` static functions
and keep only one function `initialize` and the consumer must pass
bitcoin network to which it wants to connect. This simplifies the
initialization in the dapp.
To improve the readability.
This is just a temporary interface that should be replaced by the
`OrangeKitBitcoinWalletProvider` from the `orangekit` library.
@nkuba nkuba enabled auto-merge May 29, 2024 11:34
@nkuba nkuba merged commit 3c68902 into main May 29, 2024
21 of 24 checks passed
@nkuba nkuba deleted the bitcoin-native-experience branch May 29, 2024 11:37
kkosiorowska added a commit that referenced this pull request May 29, 2024
Depends on: #371 

This PR adds support for fetching the deposits in SDK. We must fetch
data from both sources tbtc API and Acre subgraph because the tbtc API
includes additional `queued` deposits - this means they have not yet
been initialized or finalized. The subgraph only indexes initialized and
finalized deposits, so we need to combine them.
r-czajkowski added a commit that referenced this pull request May 29, 2024
… flow (#434)

Depends on: #371

This PR removes the unnecessary check condition if the `ethAccount`
exists for deposit flow. The ETH account is no longer needed, so we
should remove it from the wallet context. However, let's do it in a
separate PR.

### Implemented solution

https://github.com/thesis/acre/assets/23117945/ddec8803-c986-47bc-977d-eed11bf2a069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔌 SDK TypeScript SDK Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calculate Ethereum deposit owner address
2 participants